Skip to content

fix(deployment): join background deploy thread to stop cross-test state leak - #361

Open
deanq wants to merge 2 commits into
mainfrom
deanq/fix-resource-manager-test-pollution
Open

fix(deployment): join background deploy thread to stop cross-test state leak#361
deanq wants to merge 2 commits into
mainfrom
deanq/fix-resource-manager-test-pollution

Conversation

@deanq

@deanq deanq commented Aug 7, 2026

Copy link
Copy Markdown
Member

Summary

Fixes the intermittent CI failure

FAILED tests/unit/test_regressions.py::TestREG008NonInteractiveEnvDelete::test_undeploy_resource_force_remove_no_tty
  - _pickle.PicklingError: args[0] from __newobj__ args has the wrong class

which has been hitting a single Python version at random on unrelated PRs.

FAILED <nodeid> - <exc> means the test itself raised, not xdist failing to serialize a report — the victim test is innocent.

Root cause

  1. deploy_all_background() spawns a daemon thread and returns nothing, so callers have no handle to join.
  2. test_deploy_all_background patched get_or_deploy_resource, started the thread, and returned immediately. The patch unwound while the worker was still running, so the thread went on to hit the real deploy path with AsyncMock resources and cached them via _add_resource.
  3. ResourceManager._resources is a class variable. The late write lands in whichever dict is current when it happens — one belonging to a later test.
  4. The next test to call _save_resources() cloudpickles that leftover AsyncMock and dies.

The autouse reset_singletons fixture is already correct and function-scoped; it cannot help, because the rogue write happens after the reset. Which test pays depends on thread scheduling and xdist load distribution — hence the randomness.

Traced rather than inferred, via a plugin recording which test inserted each _resources entry:

### SAVE FAILED during test: tests/unit/test_regressions.py::TestREG008...
### CULPRIT key=<MagicMock name='mock.get_resource_key()'> type=<class 'unittest.mock.AsyncMock'>
    inserted_by=tests/unit/test_deployment.py::TestDeploymentOrchestrator::test_deploy_all_background

Changes

  • deploy_all_background returns Optional[threading.Thread]. Backwards-compatible — it previously returned None implicitly, and no production caller reads the value.
  • test_deploy_all_background joins inside the patch context, so the worker can no longer outlive the mock.
  • New test_deploy_all_background_returns_joinable_thread pins the contract and asserts the deploy mock absorbed every resource.
  • Empty-list path asserts None.
  • Separate commit resyncs uv.lock's requires-python (<3.13<3.14) with pyproject.toml; CI already runs a 3.13 job. Resolution unchanged, no package versions move.

Test plan

Written test-first; the new test failed with AttributeError: 'NoneType' object has no attribute 'join' before the fix, and the pre-fix run visibly logged the leak (caching for cleanup ×3), now absent.

  • The serial run reproduces the failure deterministically on main: 1 failed, 2629 passed → now 2618 passed, 0 failed
  • make quality-check green (format, lint, 2618 parallel + 53 serial)
  • mypy src/runpod_flash/core/deployment.py clean
  • 3 consecutive -n auto runs, all 2618 passed

@capy-ai

capy-ai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an intermittent CI failure caused by deploy_all_background() spawning a daemon thread without returning a handle to join, allowing mocked state to leak across tests via ResourceManager’s class-level cache.

Changes:

  • Update DeploymentOrchestrator.deploy_all_background() to return a joinable threading.Thread (or None for an empty resource list).
  • Update/extend unit tests to join the background deploy thread within the patch scope and to assert the new return contract.
  • Align uv.lock’s requires-python constraint with pyproject.toml by allowing Python 3.13 (<3.14).

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File Description
uv.lock Updates the lock metadata requires-python to match supported Python versions.
tests/unit/test_deployment.py Joins the returned background deploy thread in tests and adds coverage for the joinable-thread contract.
src/runpod_flash/core/deployment.py Returns an optional thread from deploy_all_background() so callers (notably tests) can join and avoid cross-test state leaks.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.


# Should not block
orchestrator.deploy_all_background(mock_resources)
thread = orchestrator.deploy_all_background(mock_resources)
deanq added 2 commits August 10, 2026 09:42
deploy_all_background spawned a daemon thread and returned nothing, so
callers had no way to wait for it. test_deploy_all_background patched
ResourceManager.get_or_deploy_resource, started the thread, and returned
immediately -- the patch unwound while the worker was still running, so
the thread went on to hit the real deploy path with AsyncMock resources
and cached them via _add_resource.

ResourceManager._resources is a class variable, and the late write lands
in whichever dict is current when it happens, i.e. one belonging to a
later test. The autouse reset_singletons fixture cannot prevent this --
the write occurs after the reset. Any test that subsequently triggers
_save_resources() dies in cloudpickle:

  _pickle.PicklingError: args[0] from __newobj__ args has the wrong class

Which test pays for it depends on thread scheduling and on how xdist
distributes work, which is why this surfaced as an intermittent failure
in test_regressions.py::TestREG008 on a single Python version.

Return the thread so callers can join it, and join it in the test inside
the patch context.

- deploy_all_background now returns Optional[threading.Thread]
- test_deploy_all_background joins before releasing its patches
- add test_deploy_all_background_returns_joinable_thread to pin the
  contract, asserting the deploy mock absorbed every resource
- assert the empty-list path returns None
pyproject.toml declares >=3.10,<3.14 and CI runs a 3.13 job, but the
committed lock still pinned >=3.10,<3.13. Any `uv sync` on 3.13
regenerated the file, leaving a spurious diff in the working tree.

Resolution is unchanged -- `uv lock` rewrites only the requires-python
line, no package versions move.
@deanq
deanq force-pushed the deanq/fix-resource-manager-test-pollution branch from cd5f32a to dc6983a Compare August 10, 2026 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants